Use block icons in media placeholders #11788
Conversation
|
Test failures seem to be completely unrelated. |
|
Tests are passing now. Thanks for restarting them, @earnjam! 🙂 |
|
They feel just a bit crowded to the text compared to the old icons. Flagged for design feedback See screenshots for comparison:
|
|
Good catch, @earnjam — the icons definitely need some additional space. Does Gutenberg not automatically provide the correct spacing between icon + label for placeholder blocks? I assumed it would take care of that kind of thing for folks. |
|
@melchoyce I had assumed it would as well. I am using the |
Should be applied at the placeholder component level. Right now it addresses svgs that are dashicons, not any svg. Should be easy, I can look into it tomorrow. |
|
Fix for it here: #11947 |
|
Rebased and resolved merge conflicts. Thanks for the margin fix PR, @mtias! 😄 |
|
Rebased and resolved merge conflicts again. |
gziolo
requested review from
kjellr,
mapk,
melchoyce,
jasmussen and
karmatosed
gziolo
added
the
[Feature] Media
There was a problem hiding this comment.
Thanks for sticking with this, @ZebulanStanphill. It's nice to get these icons all synced up.
Design-wise, this looks pretty good. Just a minor note: All of these icons seem just a little bit too low compared to the text. The icons need to be moved up 1px in order to be centered visually:
Current:
After:
I have one other question for everyone: should we actually be using an image icon within the Media + Text block here?
In this case, that placeholder area is for "Media", not specifically to "Media + Text" like the icon implies.
Thanks again!
The "Content" on the right is the text placeholder, right? |
|
@kjellr I have now updated the Media & Text block placeholder to use that new icon. 🙂 |
There was a problem hiding this comment.
On the code level, all those changes look good. I like that icons were moved to their own file 👍
I'm leaving final ✅ to @melchoyce and @kjellr who have better idea how those icons should integrate :)
There was a problem hiding this comment.
Looks great, thank you @ZebulanStanphill! I still may iterate on that image/video icon a little bit, but that can be handled separately. 👍
* Use block icons in media placeholders * Add braces around BlockIcon components when used as attribute values. Co-Authored-By: ZebulanStanphill <zebulanstanphill@gmail.com> * Vertically center content of placeholder label. * Change Media & Text block placeholder icon
* Use block icons in media placeholders * Add braces around BlockIcon components when used as attribute values. Co-Authored-By: ZebulanStanphill <zebulanstanphill@gmail.com> * Vertically center content of placeholder label. * Change Media & Text block placeholder icon
Description
Fixes #9642.
This PR changes the media placeholders of the media blocks to use the corresponding icon of each block, instead of using Dashicons. Since the icons are now being used by both the
index.jsand (if it exists)edit.jsof each of these blocks, I have moved these icons intoicon.jsfiles for each block, so the icons are not defined twice and can be imported by multiple other modules.List of blocks this PR affects:
I also took the opportunity to reorganize the imports in the files I modified: they are now organized alphabetically, and imports that were previously incorrectly listed under "Internal dependencies" are now listed under "WordPress dependencies".
Screenshot
Related issues and PRs